Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2 #2064

SamuelTrahanNOAA · 2023-12-27T19:52:21Z

Commit Queue Requirements:

Fill out all sections of this template.
All sub component pull requests have been reviewed by their code managers.
Run the full RT suite (compared to current baselines) on either Hera/Derecho/Hercules AND have committed the log to my PR branch.
Add list of all failed regression tests in "Regression Tests" section.

PR Information

Description

Fixes a bug that can crash the RRFS ensembles. When KBAS=1, there's an out-of-bounds write in an array. That corrupts memory and occasionally crashes the model.

Also, I remove from default_vars.sh the unused DO_MYJPBL variable whose value contains a typo.

Commit Message

Eliminate an out-of-bounds write in FV3 dynamical core hail diagnostic code.

FV3 component GFDL_atmos_cubed_sphere submodule:

The module_diag_hailcast.F90 has a KBAS index which can have the value of 1. When it has that value, a later line writes to address 0 of a 1-based array. This caused the RRFS ensemble to crash on WCOSS2.

Priority

Critical Bugfix (This PR contains a critical bug fix and should be prioritized.)
High (This PR contains a feature or fix needed for a time-sensitive project (eg, retrospectives, implementations))
Normal

Blocking Dependencies

Git Issues Fixed By This PR

fixes Typo when setting DO_MYJPBL to true in certain RTs #1814

Changes

Subcomponent (with links)

Input data

No changes are expected to input data.

Regression Tests:

No changes are expected to any regression test.

Libraries

Not Needed

Testing Log:

RDHPCS
- Hera
- Orion
- Hercules
- Jet
- Gaea
- Derecho
WCOSS2
- Dogwood/Cactus
- Acorn
CI
- Completed
opnReqTest
- N/A
- Log attached to comment

SamuelTrahanNOAA · 2023-12-27T20:05:19Z

This is a draft because I discovered I used the wrong fix. I need to do this instead:

-    DO k=KBAS,nz
+    DO k=KBAS+1,nz
        RWA_new(k) = RWA_new(k) / (h1d(k)-h1d(k-1))
     ENDDO

I'm testing this now on WCOSS2 and Hera.

…ather-model into hailcast-k

SamuelTrahanNOAA · 2023-12-28T02:23:49Z

The correct fix works too. This is ready for review.

jkbk2004 · 2024-01-02T17:21:17Z

@SamuelTrahanNOAA do you think we can combine this pr to @dustinswales 's #2035 ? All no baseline changes. For this pr, just need to point to the cubed-sphere branch.

SamuelTrahanNOAA · 2024-01-02T17:24:37Z

@SamuelTrahanNOAA do you think we can combine this pr to @dustinswales 's #2035 ? All no baseline changes. For this pr, just need to point to the cubed-sphere branch.

This PR needs more testing and review before it can be merged. The original developer of the code needs to confirm my fix is correct. Also, the parallels are still testing the change.

SamuelTrahanNOAA · 2024-01-02T18:38:08Z

I've changed this to a draft to reflect the fact that it needs more review.

SamuelTrahanNOAA · 2024-01-03T20:53:38Z

There are more errors in the module_diag_hailcast.F90 code than I first thought. I'm waiting to hear back from the original developer about how to fix them.

…in its value)

SamuelTrahanNOAA · 2024-01-12T16:01:41Z

@jkbk2004 - This has been ready for review for several days. I forgot to mark it so.

jkbk2004 · 2024-01-16T19:25:47Z

@SamuelTrahanNOAA can you sync up branch? wcoss2 file system maintenance this week. So we may go ahead with no baseline change PR first.

SamuelTrahanNOAA · 2024-01-16T20:16:50Z

can you sync up branch? wcoss2 file system maintenance this week. So we may go ahead with no baseline change PR first.

@jkbk2004 - Yes, it is up to date now.

jkbk2004 · 2024-01-16T20:26:37Z

@zach1221 @FernandoAndrade-NOAA @BrianCurtis-NOAA FYI: this PR is ready.

BrianCurtis-NOAA · 2024-01-17T03:16:41Z

If we're skipping WCOSS2 we can't properly test this PR, right?

jkbk2004 · 2024-01-17T13:16:27Z

If we're skipping WCOSS2 we can't properly test this PR, right?

@BrianCurtis-NOAA @SamuelTrahanNOAA I think we should move on to merge this PR even only with RDHPCS tests. Government shutdown risk is not still cleared out. In that case, GFDL feds will not be available.

SamuelTrahanNOAA · 2024-01-17T13:50:43Z

If we're skipping WCOSS2 we can't properly test this PR, right?

@BrianCurtis-NOAA @SamuelTrahanNOAA I think we should move on to merge this PR even only with RDHPCS tests. Government shutdown risk is not still cleared out. In that case, GFDL feds will not be available.

There has been some minimal testing on WCOSS with RRFS. That isn't as thorough as a regression test.

Is Acorn available?

BrianCurtis-NOAA · 2024-01-17T14:04:53Z

If we're skipping WCOSS2 we can't properly test this PR, right?

@BrianCurtis-NOAA @SamuelTrahanNOAA I think we should move on to merge this PR even only with RDHPCS tests. Government shutdown risk is not still cleared out. In that case, GFDL feds will not be available.

There has been some minimal testing on WCOSS with RRFS. That isn't as thorough as a regression test.

Is Acorn available?

@SamuelTrahanNOAA was this issue also on Acorn. I would be OK if the WCOSS2 issue is present here, and using the Acorn RT to confirm it's fixed.

SamuelTrahanNOAA · 2024-01-17T14:22:40Z

If we're skipping WCOSS2 we can't properly test this PR, right?

I think we should move on to merge this PR even only with RDHPCS tests. Government shutdown risk is not still cleared out. In that case, GFDL feds will not be available.

There has been some minimal testing on WCOSS with RRFS. That isn't as thorough as a regression test.
Is Acorn available?

was this issue also on Acorn. I would be OK if the WCOSS2 issue is present here, and using the Acorn RT to confirm it's fixed.

I'm more concerned about whether the results change for other tests.

We know this doesn't stop all the crashes. That's why Anning has another fix. There's a bug fix in the DA code. Most likely, there'll be a dozen more bug fixes before this is operational.

SamuelTrahanNOAA · 2024-01-17T16:06:12Z

I've retested it with one of the failing cases. The test was on Hera, but it fails in debug mode reliably on Hera without the fix.

The latest fix still works.

BrianCurtis-NOAA · 2024-01-17T16:11:18Z

For now, Acorn is a full pass. Not happy with these long term WCOSS2 outages

SamuelTrahanNOAA · 2024-01-17T16:43:39Z

For now, Acorn is a full pass. Not happy with these long term WCOSS2 outages

Acorn was created to lessen the impact of these outages, and also security concerns. WCOSS2's predecessors didn't have a mid-sized test system. You are witnessing the Acorn test machine serving its intended purpose.

jkbk2004 · 2024-01-17T17:00:42Z

OK! I think we can move on for merging process. @SamuelTrahanNOAA I am moving to NOAA-GFDL/GFDL_atmos_cubed_sphere#308 to ask merging there.

jkbk2004 · 2024-01-17T19:12:32Z

@SamuelTrahanNOAA FV3 pr was merged.

SamuelTrahanNOAA · 2024-01-17T19:21:47Z

I've reverted the .gitmodules changes and updated the FV3 hash to NOAA-EMC develop's head.

This PR is ready for final review and merge.

SamuelTrahanNOAA added 4 commits December 23, 2023 21:24

fix out-of-bounds write in module_diag_hailcast

dd9262c

point to sam's branch

2ce5344

better fix for out-of-bounds write in module_diag_hailcast

e27a95a

hera tests passed

4e29477

SamuelTrahanNOAA changed the title ~~Fix out-of-bounds write in module_diag_hailcast.F90 which crashes RRFS on WCOSS2~~ Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2 Dec 27, 2023

SamuelTrahanNOAA marked this pull request as draft December 27, 2023 20:05

SamuelTrahanNOAA added 3 commits December 27, 2023 21:51

change to the correct bug fix

f60ead3

Merge branch 'hailcast-k' of ssh://github.com/SamuelTrahanNOAA/ufs-we…

aef3ed5

…ather-model into hailcast-k

hera tests passed again

042090f

SamuelTrahanNOAA marked this pull request as ready for review December 28, 2023 02:23

SamuelTrahanNOAA marked this pull request as draft January 2, 2024 18:37

remove unused variable DO_MYJPBL from default_vars.sh (it has a typo …

117d328

…in its value)

SamuelTrahanNOAA mentioned this pull request Jan 4, 2024

Typo when setting DO_MYJPBL to true in certain RTs #1814

Closed

SamuelTrahanNOAA added 3 commits January 4, 2024 18:00

only correct RWA_new where RWA_adiabat(k).ge.1.E-12

e44a6cf

module_diag_hailcast.F90: update comments; changes from @adams-selin

65bb581

hera tests passed again

382856a

SamuelTrahanNOAA marked this pull request as ready for review January 12, 2024 16:01

merge develop

a3fd48c

jkbk2004 added No Baseline Change No Baseline Change Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked. labels Jan 16, 2024

zach1221 and others added 2 commits January 16, 2024 22:23

add hercules RT logs: passed

8cde752

add gaeaC5 log passed

386d7e3

zach1221 added 4 commits January 17, 2024 08:08

add orion RT logs: passed

15b6647

add control_p8_gnu ORT logs: passed

b86f5cd

add regional_control_gnu ORT logs: passed

68b65b6

add cpld_control_gnu ORT logs: passed

a0c4f65

Acorn RT Log

80ba535

add jet RT log passed

7eaf1a4

jkbk2004 mentioned this pull request Jan 17, 2024

Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2. NOAA-GFDL/GFDL_atmos_cubed_sphere#308

Merged

6 tasks

SamuelTrahanNOAA added 2 commits January 17, 2024 19:20

FV3 hash points to head of develop

c0254e4

revert .gitmodules

4f94c13

jkbk2004 requested review from SadeghTabas-NOAA, BrianCurtis-NOAA, zach1221 and FernandoAndrade-NOAA January 17, 2024 19:25

BrianCurtis-NOAA approved these changes Jan 17, 2024

View reviewed changes

zach1221 approved these changes Jan 17, 2024

View reviewed changes

jkbk2004 merged commit e7380dd into ufs-community:develop Jan 17, 2024

grantfirl mentioned this pull request Mar 14, 2024

UFS-dev PR#94 NCAR/ufs-weather-model#126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2 #2064

Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2 #2064

SamuelTrahanNOAA commented Dec 27, 2023 •

edited by jkbk2004

Loading

SamuelTrahanNOAA commented Dec 27, 2023

SamuelTrahanNOAA commented Dec 28, 2023

jkbk2004 commented Jan 2, 2024

SamuelTrahanNOAA commented Jan 2, 2024

SamuelTrahanNOAA commented Jan 2, 2024

SamuelTrahanNOAA commented Jan 3, 2024

SamuelTrahanNOAA commented Jan 12, 2024

jkbk2004 commented Jan 16, 2024

SamuelTrahanNOAA commented Jan 16, 2024

jkbk2004 commented Jan 16, 2024

BrianCurtis-NOAA commented Jan 17, 2024

jkbk2004 commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

BrianCurtis-NOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

BrianCurtis-NOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

jkbk2004 commented Jan 17, 2024 •

edited

Loading

jkbk2004 commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2 #2064

Fix out-of-bounds access in module_diag_hailcast.F90 which crashes RRFS on WCOSS2 #2064

Conversation

SamuelTrahanNOAA commented Dec 27, 2023 • edited by jkbk2004 Loading

Commit Queue Requirements:

PR Information

Description

Commit Message

Priority

Blocking Dependencies

Git Issues Fixed By This PR

Changes

Subcomponent (with links)

Input data

Regression Tests:

Libraries

Testing Log:

SamuelTrahanNOAA commented Dec 27, 2023

SamuelTrahanNOAA commented Dec 28, 2023

jkbk2004 commented Jan 2, 2024

SamuelTrahanNOAA commented Jan 2, 2024

SamuelTrahanNOAA commented Jan 2, 2024

SamuelTrahanNOAA commented Jan 3, 2024

SamuelTrahanNOAA commented Jan 12, 2024

jkbk2004 commented Jan 16, 2024

SamuelTrahanNOAA commented Jan 16, 2024

jkbk2004 commented Jan 16, 2024

BrianCurtis-NOAA commented Jan 17, 2024

jkbk2004 commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

BrianCurtis-NOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

BrianCurtis-NOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

jkbk2004 commented Jan 17, 2024 • edited Loading

jkbk2004 commented Jan 17, 2024

SamuelTrahanNOAA commented Jan 17, 2024

SamuelTrahanNOAA commented Dec 27, 2023 •

edited by jkbk2004

Loading

jkbk2004 commented Jan 17, 2024 •

edited

Loading